xds/clusterresolver: implement gRFC A61 changes for LOGICAL_DNS clusters#8733
xds/clusterresolver: implement gRFC A61 changes for LOGICAL_DNS clusters#8733Pranjali-2501 merged 12 commits intogrpc:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8733 +/- ##
==========================================
- Coverage 83.42% 83.35% -0.07%
==========================================
Files 414 414
Lines 32751 32754 +3
==========================================
- Hits 27322 27302 -20
- Misses 4040 4057 +17
- Partials 1389 1395 +6
🚀 New features to boost your workflow:
|
|
This change probably needs a release note. |
internal/xds/balancer/clusterresolver/e2e_test/dns_impl_test.go
Outdated
Show resolved
Hide resolved
| localityStr := xdsinternal.LocalityString(clients.Locality{}) | ||
| retEndpoint = hierarchy.SetInEndpoint(retEndpoint, []string{pName, localityStr}) | ||
| // Set the locality weight to 1. This is required because the child policy | ||
| // like wrr which relies on locality weights to distribute traffic. These |
There was a problem hiding this comment.
This is also weighted_target (which has a parent policy of wrr_locality, whose only responsibility is to build the config for weighted_target) and not wrr (which is weighted_round_robin and is a completely different policy).
| endpoints[i] = resolver.Endpoint{Addresses: []resolver.Address{a}} | ||
| endpoints[i].Attributes = a.BalancerAttributes | ||
| } | ||
| endpoints = []resolver.Endpoint{{Addresses: state.Addresses}} |
There was a problem hiding this comment.
@arjan-bal recently made a change #8812 to ensure that the DNS resolver returns endpoints. So, I think we don't need a special case here for len(endpoints) == 0. Please correct me if I'm wrong.
There was a problem hiding this comment.
Right, we don't need that special case.
| Cluster: testClusterName2, | ||
| ChildPolicy: tt.xdsLBPolicy, | ||
| } | ||
| if diff := cmp.Diff(gotConfig, wantConfig); diff != "" { |
There was a problem hiding this comment.
When using cmp.Diff, it makes more sense to pass the want as the first argument and the got as the second. And in the error string have (-want, +got). When we do this, and see an error string with some lines which have a prefix of -, we can say that those lines were expected but missing in the output. And similarly, when we see some lines with a prefix of +, we can say that those lines were not expected, but were seen in the output. See go/go-style/decisions#types-of-equality
Here and elsewhere where cmp.Diff is used in this PR.
| server2 := stubserver.StartTestService(t, nil) | ||
| defer server2.Stop() | ||
|
|
||
| // Register a manual resolver with the "dns" scheme to mock DNS resolution. |
There was a problem hiding this comment.
resolve conflicts
…ers (grpc#8733) Fixes : grpc#8153 This PR implements the "Changes to Logical DNS Clusters" section of [gRFC A61 (IPv4/IPv6 Dual-stack Backends)](https://github.com/grpc/proposal/blob/master/A61-IPv4-IPv6-dualstack-backends.md#changes-to-logical-dns-clusters). Currently, `LOGICAL_DNS` clusters in the xDS cluster resolver have their Load Balancing policy hard-coded to `pick_first`. This ensures the semantics of connecting to only one address at a time. This PR updates the `xds_cluster_resolver` logic. The `buildClusterImplConfigForDNS` function removes the hard-coded `pick_first` policy restriction for `LOGICAL_DNS` clusters, allowing them to use the configured LB policy. Also, in the DNS update handler, all resolved addresses are now grouped into a single `resolver.Endpoint`. This ensures that regardless of the configured parent LB policy, the child policy sees a single "backend" endpoint containing all addresses. RELEASE NOTES: * xds: `LOGICAL_DNS` clusters now honor the LB policy configured in the cluster resource, rather than defaulting to a hardcoded `pick_first` policy.
…ers (grpc#8733) Fixes : grpc#8153 This PR implements the "Changes to Logical DNS Clusters" section of [gRFC A61 (IPv4/IPv6 Dual-stack Backends)](https://github.com/grpc/proposal/blob/master/A61-IPv4-IPv6-dualstack-backends.md#changes-to-logical-dns-clusters). Currently, `LOGICAL_DNS` clusters in the xDS cluster resolver have their Load Balancing policy hard-coded to `pick_first`. This ensures the semantics of connecting to only one address at a time. This PR updates the `xds_cluster_resolver` logic. The `buildClusterImplConfigForDNS` function removes the hard-coded `pick_first` policy restriction for `LOGICAL_DNS` clusters, allowing them to use the configured LB policy. Also, in the DNS update handler, all resolved addresses are now grouped into a single `resolver.Endpoint`. This ensures that regardless of the configured parent LB policy, the child policy sees a single "backend" endpoint containing all addresses. RELEASE NOTES: * xds: `LOGICAL_DNS` clusters now honor the LB policy configured in the cluster resource, rather than defaulting to a hardcoded `pick_first` policy.
Fixes : #8153
This PR implements the "Changes to Logical DNS Clusters" section of gRFC A61 (IPv4/IPv6 Dual-stack Backends).
Currently,
LOGICAL_DNSclusters in the xDS cluster resolver have their Load Balancing policy hard-coded topick_first. This ensures the semantics of connecting to only one address at a time.This PR updates the
xds_cluster_resolverlogic. ThebuildClusterImplConfigForDNSfunction removes the hard-codedpick_firstpolicy restriction forLOGICAL_DNSclusters, allowing them to use the configured LB policy.Also, in the DNS update handler, all resolved addresses are now grouped into a single
resolver.Endpoint. This ensures that regardless of the configured parent LB policy, the child policy sees a single "backend" endpoint containing all addresses.RELEASE NOTES:
LOGICAL_DNSclusters now honor the LB policy configured in the cluster resource, rather than defaulting to a hardcodedpick_firstpolicy.